Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix and Revert "Revert "Display banner to enable 2FA when setting up VBBA"" #14029

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Fix and Revert "Revert "Display banner to enable 2FA when setting up VBBA"" #14029

merged 9 commits into from
Feb 2, 2023

Conversation

MonilBhavsar
Copy link
Contributor

@MonilBhavsar MonilBhavsar commented Jan 5, 2023 •
edited by youssef-lr
Loading

Details

Reverts and updates to the previous PR to fix the functionality

Reverts #13407

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/238668

Tests

Let user have 2FA disabled
Tests with https://github.com/Expensify/Web-Expensify/pull/35988.

  1. Checkout that branch
  2. Run app
  3. Go to Workspace > Connect bank account
  4. Add a bank account in a PENDING state, go till Validation step and confirm the 2FA prompt
  5. Click on secure account and confirm it redirects to olddot, enables 2FA.
  6. Finish the setup and confirm it redirects back to newdot and this time it has no prompt
  7. Go to olddot and disable 2FA
  8. I manually changed account state, to go through validation state and confirmed the 2FA prompt there

Notes:

  1. On iOS safari, you need to go to settings > Safari > Disable block popups
  2. On iOS Safari, update settings to request desktop site always to load olddot
  3. Change URL_TO_NEWDOT in dev-config on olddot to reflect ngrok url instead of localhost:8080 (localhost doesn't work on android chrome)
  4. Change EXPENSIFY_URL in App .env to reflect your ngrok url so that the linking from App to olddot works.
  • Verify that no errors appear in the JS console

Offline tests

Doesn't work offline

QA Steps

Prerequisite: User should have 2FA disabled in olddot > Settings > Accounts

  1. Go to Workspace > Connect bank account and start the process of setting up bank account in VERIFYING state
  2. Go till Verifying state and confirm the 2FA prompt as in screenshot
  3. Click on "Secure Account" and confirm you're redirected to olddot and 2FA modal is open.
  4. Finish the 2FA setup. Upon clicking Finish, confirm you're redirected back to newdot
  5. Confirm this time there is not any 2FA prompt.

Now,

  1. Disable 2FA from olddot
  2. Go to bank account setup again. Go to Workspace > Connect bank account
  3. Wait till Bank account is verified and you go to validation step and see the inputs to enter 3 amounts
  4. Confirm 2FA prompt as in screenshot
  5. Click on "Secure Account" and confirm you're redirected to olddot and 2FA modal is open.
  6. Finish the 2FA setup. Upon clicking Finish, confirm you're redirected back to newdot
  7. Confirm this time there is not any 2FA prompt.
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

23/01

Screen.Recording.2023-01-23.at.4.29.37.PM.mov
Web
Screen.Recording.2023-01-06.at.10.41.19.AM.mov
Mobile Web - Chrome
Screen.Recording.2023-01-06.at.6.41.56.PM.mov
Mobile Web - Safari
Screen.Recording.2023-01-06.at.5.56.57.PM.mov
Desktop
Screen.Recording.2023-01-06.at.4.12.35.PM.mov
iOS
Screen.Recording.2023-01-06.at.6.01.38.PM.mov
Android
Screen.Recording.2023-01-06.at.7.18.10.PM.mov

@MonilBhavsar MonilBhavsar self-assigned this Jan 5, 2023
@MonilBhavsar MonilBhavsar changed the title Fix and Revert "Revert "Display banner to enable 2FA when setting up VBBA"" [Hold Web-E 35988] Fix and Revert "Revert "Display banner to enable 2FA when setting up VBBA"" Jan 6, 2023
@MonilBhavsar MonilBhavsar marked this pull request as ready for review January 6, 2023 10:49
@MonilBhavsar MonilBhavsar requested a review from a team as a code owner January 6, 2023 10:49
@melvin-bot melvin-bot bot requested review from dangrous and mananjadhav and removed request for a team January 6, 2023 10:49
@melvin-bot
Copy link

melvin-bot bot commented Jan 6, 2023

@mananjadhav @dangrous One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@mananjadhav
Copy link
Collaborator

@MonilBhavsar Is the API available on staging to test and review?

@MonilBhavsar
Copy link
Contributor Author

Not until, we deploy the Web PR. Will notify here, once it is deployed and this PR is unheld

@dangrous
Copy link
Contributor

dangrous commented Jan 6, 2023

While we're waiting for the Web-E pull, a quick question - it looks like there are two places where this isn't quite a revert:

Am I interpreting that right? Totally fine either way but want to make sure I'm not missing anything. Thanks!

@MonilBhavsar
Copy link
Contributor Author

@dangrous yes correct! This is not a straight revert, in fact a revert with some amendments. Looks like you got the changes that differs from the previous PR that was reverted 😄
Also, Web-E PR was merged, so you'll be able to test by checking out main. Please make sure to run grunt. Thanks!

@dangrous
Copy link
Contributor

dangrous commented Jan 9, 2023

Hey! So tested this on newDot web and got through setting up 2FA on oldDot. However when I click the "Finish" button on oldDot after setting it up, instead of redirecting back to the App it goes to https://www.expensify.com.dev/bank-account/ which is a 404 error. Is this known?

The app side of things, though, DOES work - the prompt appears if 2FA is not enabled, and disappears if it is (on a page reload). Haven't tested the other platforms yet due to the above.

@MonilBhavsar
Copy link
Contributor Author

@mananjadhav you'll be now able to test on staging

@MonilBhavsar
Copy link
Contributor Author

Hey! So tested this on newDot web and got through setting up 2FA on oldDot. However when I click the "Finish" button on oldDot after setting it up, instead of redirecting back to the App it goes to https://www.expensify.com.dev/bank-account/ which is a 404 error. Is this known?

Hmm, not known. Could you please confirm, you have checked out main, ran grunt on Web-E.
And URL_TO_NEW_DOT in dev config points to a Newdot URL.

@mananjadhav
Copy link
Collaborator

Okay thanks. I'll get to this today.

@dangrous
Copy link
Contributor

@MonilBhavsar Apologies, I thought I had run grunt but maybe I didn't. Works like a charm now! I'll wait for @mananjadhav to confirm the testing but looking good and we can merge shortly.

@mananjadhav
Copy link
Collaborator

I just testing this and should have the checklist in 30mins-1 hour.

dangrous
dangrous previously approved these changes Jan 10, 2023
View reviewed changes
mananjadhav
mananjadhav previously approved these changes Jan 10, 2023
View reviewed changes
@mananjadhav
Copy link
Collaborator

I am a little blocked for the testing, @MonilBhavsar @dangrous. Can you help me what can be done here? I tried adding. multiples non public emails, and refresh. It blocks me later for multiple secondary attempts. Can't get past this step.

image

@dangrous
Copy link
Contributor

So I think that might be a different flow - are you adding a bank account in a VERIFYING state? I think that way you won't hit that screen. I realize now that that instruction is in the QA Steps but not testing, I'll update the description quickly.

@mananjadhav
Copy link
Collaborator

@dangrous I am not sure what you mean by adding the bank account in Verifying State.

Here's the steps that I am following:

  1. Go to Settings -> Workspaces
  2. Choose any workspace -> Connect bank account
  3. Connect via Plaid
  4. Followed the steps to add the account
  5. Takes me to Company Information
  6. Next Personal Information
  7. Next Additional Information, and on continue I get stuck on this screen.

@MonilBhavsar
Copy link
Contributor Author

Ah yes, this needs to be tested internally because of bank accounts, I think

@MonilBhavsar
Copy link
Contributor Author

No worries!

@dangrous I tried again today, in a different browser and could not reproduce that issue

Screen.Recording.2023-01-20.at.11.24.21.PM.mov

@MonilBhavsar
Copy link
Contributor Author

MonilBhavsar commented Jan 20, 2023 •
edited
Loading

However, a question - this flow shouldn't actually work right now for real users - there's no way for a user to scan the QR code to get 2FA set up, except if they have two devices. Is this an issue, and/or is it okay to skip this testing altogether?

Correct, also olddot doesn't work on mobile web, unless one selects "Request a desktop" site, but it would be good to test it as a part of process.

iOS and Android both redirect the user to newDot in the browser, rather than the app itself. This would be a Web-E issue, do we know if it's being worked on?

Yes, in the physical, I think there is an option in android and iOS to open that in the app.

@dangrous
Copy link
Contributor

dangrous commented Jan 20, 2023 •
edited
Loading

Okay cool - so how do you want to move forward? Should I fill out the checklist with the caveat that I was unable to confirm the behavior on mobile web, but that it won't be a flow that needs to work for the end user? Or are we able to get someone else to test? What do you think?

I'm hoping the desktop flakiness is just on my end, but would be great to have one more test of that too, since I have no idea why it's happening for me

(Also merge conflicts now, sorry!)

@MonilBhavsar
Copy link
Contributor Author

I think we should test it. @mountiny @youssef-lr if you could help us test and complete the checklist

@youssef-lr
Copy link
Contributor

Testing now!

@dangrous
Copy link
Contributor

Thanks for jumping in @youssef-lr ! Sorry my testing keeps causing so much trouble

@mountiny
Copy link
Contributor

Have you had success? @youssef-lr I will try but not sure if I will get around to this, gotta deploy web today

@youssef-lr
Copy link
Contributor

youssef-lr commented Jan 23, 2023 •
edited
Loading

Sorry @mountiny, I had some issues in my VM after updating everything, and had to do a destroy+up, it's now working properly and I'm back to testing! Will report back here as soon as I'm done.

@youssef-lr
Copy link
Contributor

Testing on iOS is working as expected! I added a 4th step to the notes in the issue description as I needed to do this for linking to work.

4. Change EXPENSIFY_URL in App .env to reflect your ngrok url so that the linking from App to olddot works.

RPReplay_Final1674494460.2.MP4

Will update this comment with Chrome Android results.

@mountiny
Copy link
Contributor

Oh nice, thanks @youssef-lr

@MonilBhavsar if we got that one from Youssef, seems like we might be able to merge this one, right?

@youssef-lr
Copy link
Contributor

Seems like the issue @dangrous was originally having occurs on Android Chrome, when I'm taken back to NewDot, I find myself logged out.

Screen.Recording.2023-01-23.at.20.06.31.mov

@dangrous
Copy link
Contributor

Hmm @MonilBhavsar any thoughts? On the one hand I'm happy that it's not just me, but on the other hand, now we have to figure out why haha

@mountiny
Copy link
Contributor

Crazy thought, should we add the 2FA set up to NewDot right now, I mean it will have to be added eventually and probably quite soon. These bridges over to oldDot and back will always been a bit wobly.

Or, we can also deploy this to staging, keep an eye on it, let QA test this out wild on all platforms and revert in case it does not work. Since we can see it works for some and less for others it might be due to the fact that this portal between New and Old dot is not that great in dev. If @MonilBhavsar tested this and it worked for him and it worked fine for almost all platforms to the other reviewers, then we should be fine getting this out. Thoughts?

@dangrous
Copy link
Contributor

I might agree with you @mountiny given that the tested flow is not actually a viable flow except on desktop - all mobile implementations, whether mWeb or the app, require the user to request the desktop app and then use a different device to scan the QR code, which is.... very unlikely. So as long as we're sure desktop (definitely works) and desktop web (mostly works?) work, I think we would be okay.

It does bother me that we don't yet know the source of the flakiness, but maybe that's a separate issue?

@mountiny
Copy link
Contributor

It does bother me that we don't yet know the source of the flakiness, but maybe that's a separate issue?

Seems like at the start the QA/Tests/ set up steps were not clear or not everyone had the same environment set up, I feel like that might have contirbuted.

I feel like we should move on, lets merge this and ping QA to specially test this flow on all platforms once in staging to ensure its working. Its not extremely used flow so I feel like this should work out.

@dangrous
Copy link
Contributor

dangrous commented Feb 1, 2023

Cool, I think that makes sense for now! @mountiny I'll let you do the honors.

mountiny
mountiny approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MonilBhavsar Are you happy with the plan? Would you be able to contact QA when this would be merged and deployed?

If this worked for you locally on mobile web as well, I believe we can go ahead. Feel free to self-merge once you happy with this!

@MonilBhavsar
Copy link
Contributor Author

Hey! Sorry for some delay here. back from some ooo.
I am having a sync today with @youssef-lr to figure out what's wrong with android, since it worked for youssef on all platforms (except android)

@MonilBhavsar
Copy link
Contributor Author

we tested yesterday and it worked for @youssef-lr
I tested again after building everything and it worked for me too. So merging it

2FA.testing.720p.mov

@MonilBhavsar MonilBhavsar merged commit e986cc8 into main Feb 2, 2023
@MonilBhavsar MonilBhavsar deleted the revert-13407-revert-12424-monil-VBA2FASetup branch February 2, 2023 06:46
@OSBotify
Copy link
Contributor

OSBotify commented Feb 2, 2023

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.2.64-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@thienlnam thienlnam mentioned this pull request Feb 2, 2023
Closed
30 tasks
@kbecciv kbecciv mentioned this pull request Feb 3, 2023
Closed
6 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

1 similar comment
@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mountiny mountiny mountiny approved these changes

@dangrous dangrous dangrous left review comments

@mananjadhav mananjadhav mananjadhav left review comments

@youssef-lr youssef-lr Awaiting requested review from youssef-lr

Assignees

@MonilBhavsar MonilBhavsar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MonilBhavsar @mananjadhav @dangrous @mountiny @youssef-lr @OSBotify